feat: distribute the Open Flow command release - #335

src/adapters/completion/static-completion-renderer.test.ts (1)

62-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import onlineDevEndpoint instead of hardcoding "oomol.dev".

createCliCatalog decides visibility with endpoint !== onlineDevEndpoint (src/application/commands/catalog.ts line 54). This test hardcodes the literal "oomol.dev". If the constant changes, the production catalog and this test drift apart.

Import the constant so the test tracks the single definition.

♻️ Proposed change

-        const devOutput = renderer.render("fish", createCliCatalog("oomol.dev"));
+        const devOutput = renderer.render("fish", createCliCatalog(onlineDevEndpoint));

Add the import alongside the existing catalog import, using the module that defines onlineDevEndpoint.

As per coding guidelines: "Never duplicate constant values across files. Define once, import or re-export with aliases elsewhere."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/adapters/completion/static-completion-renderer.test.ts` around lines 62 -
70, Update the test around StaticCompletionRenderer to import onlineDevEndpoint
from the module that defines it and pass that constant to createCliCatalog
instead of the hardcoded "oomol.dev" value, keeping the visibility assertions
unchanged.

Source: Coding guidelines

src/application/commands/flow-artifact.test.ts (2)

119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for tar path traversal rejection.

The suite covers the typeflag check with a link entry. It does not cover isNormalizedArtifactPath, which is the control that stops an archive entry from writing outside the extraction directory.

Add cases that assert rejection for an entry path containing a .. segment and for an absolute path. Both go through the same encodeTarGzip helper already in this file.

💚 Suggested additional cases

+    test("rejects tar entries that escape the artifact root", async () => {
+        const archive = encodeTarGzip([{
+            body: new Uint8Array(),
+            mode: 0o644,
+            path: "open-flow-command/../escape.js",
+            type: "0",
+        }]);
+        const release = createRelease(archive);
+        const environment = await createTestEnvironment();
+
+        await expect(installOpenFlowCommandRelease(release, {
+            env: environment.env,
+            execPath: process.execPath,
+            fetcher: createArchiveFetcher(archive),
+        })).rejects.toThrow("invalid file path");
+    });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.test.ts` around lines 119 - 134, Add
test coverage in the flow artifact installation tests for
isNormalizedArtifactPath by creating archives with one entry using a path
containing a ".." segment and another using an absolute path. Reuse
encodeTarGzip and the existing installOpenFlowCommandRelease assertion pattern,
verifying both cases reject before extraction.

---

136-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The concurrency test passes even when no serialization occurs.

The test resolves resumeRequest immediately after creating second. Nothing guarantees that second reaches acquireDownloadTempLock before first completes its install. If first finishes first, second takes the cache-hit return at flow-artifact.ts line 133 and never contends the lock. requestCount is still 1 and both assertions still pass.

The test therefore cannot distinguish lock serialization from plain cache reuse.

To make the assertion meaningful, hold the first request open until second has demonstrably started, then release:

💚 Suggested restructure

         const first = installOpenFlowCommandRelease(fixture.release, options);
         await requestStarted.promise;
         const second = installOpenFlowCommandRelease(fixture.release, options);
-        resumeRequest.resolve();
+        // Give `second` time to reach the lock while the first download is still blocked.
+        await Bun.sleep(50);
+        expect(requestCount).toBe(1);
+        resumeRequest.resolve();

This proves the second call did not issue its own request while the first held the lock.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.test.ts` around lines 136 - 166,
Restructure the test “serializes concurrent installation of the same digest” so
the first fetch remains blocked until the second installation has demonstrably
started and attempted to contend with the download lock. Add an explicit
synchronization signal for the second call’s fetch/lock-entry path, await that
signal before resolving resumeRequest, then retain the assertions that both
calls share the directory and requestCount is one.

src/application/commands/flow-artifact.ts (3)

133-135: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Every oo flow invocation re-hashes the whole cached artifact.

Line 133 calls validCommandArtifactDirectory before any lock is taken, on the normal cache-hit path. That runs validateCommandArtifactDirectory, which walks the directory tree and then reads and SHA-256 hashes every manifest-listed file (lines 651-654). The pinned release archive is about 5 MB, so each oo flow invocation pays a full read and hash of the expanded artifact before Open Flow starts.

The cache directory is already named by the archive digest and is written through an atomic rename from a private extraction directory, so a correct cache entry cannot be partially written. The full re-hash defends only against post-install local tampering or disk corruption.

Consider a cheaper steady-state check and keep the full verification for the repair path. Options:

• Compare the file set plus each file's size and mtime against a stamp written at install time, and fall back to full hashing only on mismatch.
• Write a .verified marker containing the digest after a successful install, and validate fully only when the marker is absent.

♻️ Sketch: fast path first, full validation as fallback

-    if (await validCommandArtifactDirectory(commandDirectory, release)) {
+    if (await cachedArtifactLooksIntact(commandDirectory, release)) {
         return commandDirectory;
     }

Add a helper that compares the recorded file set, sizes, and mtimes, and only calls validateCommandArtifactDirectory when that comparison fails.

Also applies to: 629-655

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.ts` around lines 133 - 135, Update the
cache-hit logic around validCommandArtifactDirectory so normal oo flow
invocations use a cheap integrity check first, comparing the recorded artifact
file set, sizes, and mtimes from installation; invoke
validateCommandArtifactDirectory only when that check fails or no install marker
exists. Preserve the existing full validation and repair behavior for
mismatches, and apply the same fast-path handling to the related validation flow
near the artifact install logic.

---

219-291: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add a timeout or idle-progress guard to downloadCommandArchive.

Fetcher and createRetryingFetcher do not impose a deadline. A stalled response can block oo flow indefinitely while holding the download lock. Pass an AbortSignal deadline or add an idle-byte watchdog, consistent with the self-update download path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.ts` around lines 219 - 291, Update
downloadCommandArchive to enforce a download deadline or idle-progress timeout,
using the same timeout/watchdog approach as the self-update download path. Apply
it to the fetch request and ensure stalled responses abort and release the
reader and file handle instead of blocking indefinitely.

---

363-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid coupling archive acceptance to local compressor output.

downloadCommandArchive checks the complete response length and SHA-256 digest before decodeCommandArchive runs. The full gzipSync(tar, { level: 9 }) comparison adds a compressor-output constraint, not archive integrity. Different node:zlib implementations or versions can reject the pinned archive with "not canonically encoded". Replace it with an implementation-independent check for trailing or truncated gzip data if that validation remains required.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.ts` around lines 363 - 380, Update
decodeCommandArchive and canonicalGzip so archive acceptance no longer depends
on local gzipSync output; retain the existing full-length and SHA-256 validation
in downloadCommandArchive, and replace the compressor comparison with an
implementation-independent check that rejects truncated or trailing gzip data if
needed.

src/application/bootstrap/run-cli.ts (1)

338-344: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Report the real delegated argument count.

argCount and flagsCount are always 0, even when openFlowInvocation.args is not empty. The flow telemetry event therefore reports every invocation as argument-free, and the dimension carries no signal. An argument count is a count, not free-form input, so it stays privacy safe. Prefer a bucket helper from src/application/telemetry/buckets.ts if one exists for counts.

♻️ Proposed change

             telemetryRecorder.observer.onCommandResolved?.({
-                argCount: 0,
+                argCount: openFlowInvocation.args.length,
                 commandPath: ["flow"],
                 excludeFromTelemetry: false,
-                flagsCount: 0,
+                flagsCount: openFlowInvocation.args.filter(
+                    argument => argument.startsWith("-"),
+                ).length,
                 outputFormat: "text",
             });

As per coding guidelines: "For useful command-specific dimensions, call context.telemetry?.recordProperties(...) from the command handler with only low-cardinality, privacy-safe enums, booleans, counts, or buckets."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/bootstrap/run-cli.ts` around lines 338 - 344, Update the flow
telemetry payload in the command-resolution path to report the real delegated
argument count from openFlowInvocation.args instead of hardcoding argCount to
zero, and derive flagsCount from the delegated flags when available. Reuse the
existing count-bucketing helper from buckets.ts if applicable, while preserving
the privacy-safe, low-cardinality telemetry contract.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/application/self-update/uninstall.test.ts (1)

203-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use resolveOpenFlowCommandCacheRoot for the expected path.

This test duplicates the platform and cache-format rules from resolveOpenFlowCommandCacheRoot. Build the expected value with that resolver and the same environment, home directory, and platform used to build the uninstall plan.

Proposed fix

-        expect(paths(userData)).toContain(join(
-            tempHome,
-            ".cache",
-            "oo",
-            "open-flow",
-            "command-artifact-v1",
-        ));
+        expect(paths(userData)).toContain(
+            resolveOpenFlowCommandCacheRoot({
+                env: { HOME: tempHome },
+                homeDirectory: tempHome,
+                platform: "linux",
+            }),
+        );

As per coding guidelines, replace test expressions that duplicate extracted production logic with the shared utility.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/self-update/uninstall.test.ts` around lines 203 - 209, Update
the expected cache path assertion in the uninstall test to use
resolveOpenFlowCommandCacheRoot with the same environment, home directory, and
platform passed when constructing the uninstall plan, instead of manually
joining cache path segments. Keep the assertion verifying that paths(userData)
contains the resolver’s returned value.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/application/commands/flow-artifact.ts (1)

266-305: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Cancel the response body reader on the failure path.

The inner finally calls reader.releaseLock() but never cancels the stream. If invalid(...) throws at Line 275 or Line 289, the remaining body is left unread and the connection is held until the abort timer fires at commandArchiveDownloadTimeoutMs. Cancel the reader so the socket is released immediately.

♻️ Proposed change to cancel the reader before releasing the lock

         finally {
-            reader.releaseLock();
+            await reader.cancel().catch(() => {});
+            reader.releaseLock();
             await fileHandle.close();
         }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/flow-artifact.ts` around lines 266 - 305, Update the
download cleanup in the inner finally block around reader and fileHandle so the
response body reader is cancelled before releaseLock(), ensuring failures from
invalid(...) terminate the remaining stream immediately while preserving the
existing file close behavior.